Review — PR #881 (round 1)

No confidentiality issues: the changeset, comments and PR body name no company other than Sapiom, no internal hosts or private links, and #809 is a bare cross-reference. Changeset level (@sapiom/harness patch) is right — internal startup ordering, no public API or type surface touched.

Findings

1. The ordering guarantee does not hold when backfill hits its per-boot cap (packages/harness/src/server/index.ts:2980, :3011)

backfillSessionRecords stops archiving at RECORDS_BACKFILL_MAX = 200, counts the rest into remaining, calls onCapped and resolves normally (core/record-archive.ts:563-573). runNdjsonSweep treats that resolution as "everything is archived" and sweeps.

Failure scenario: first boot after upgrade on an install with 250 unarchived conversations, all older than 30 days. 200 are archived; the sweep then drops the remaining 50 conversations' events by the age cap; the "left for the next boot" pass finds nothing to read. Those 50 are lost permanently — the exact outcome the PR is meant to prevent, on precisely the installs with the largest history.

Fix: have the sweep observe the cap — e.g. resolve recordBackfill to whether it completed and skip the ndjson sweep for this boot when it was capped (the size cap will still be enforced by a later tick once the backlog drains).

2. A stalled backfill now disables the retention caps for the process lifetime (packages/harness/src/server/index.ts:3010-3016)

Every sweep — boot pass and all 6-hourly ticks — is chained onto the single recordBackfill promise. Rejection is covered (recordBackfill ends in .catch, so it always settles), but a hang is not: one readFromEvents or archive.write that never settles (network-mounted ~/.sapiom, a pathological log) means sweepNdjson never runs again, and events.ndjson grows past the 50 MB cap unbounded. Before this PR the sweep had an independent path to running.

A bounded wait (Promise.race([recordBackfill, timeout]), logging when the timeout wins) keeps the new ordering without trading a hard cap for a soft one.

3. New assertion uses vi.waitFor's 1 s default and will flake (packages/harness/src/server/record-archive-wiring.test.ts:294-296)

await vi.waitFor(async () => {
  expect(await readFile(eventStorePath, "utf8")).not.toContain("sess-legacy");
});

No timeout/interval, so this is 1 000 ms. The preceding waitFor only proves the archive write landed; the ndjson sweep is still behind recordArchive.sweep() plus the store's exclusive queue at that point. Under CI load the sweep can miss the 1 s window and fail the very test that guards this behaviour. Pass { timeout: 10_000, interval: 100 }, matching the sibling waitFor three lines above.

Verdict

Right diagnosis and the ordering change is the correct shape, but request changes: finding 1 leaves the history-loss path open for backlogs over 200 while the code comment and changeset assert it is closed, and finding 2 makes retention dependent on backfill always settling.